fix for entry data field race condition #1229
Conversation
| @@ -212,68 +224,68 @@ func (entry Entry) HasCaller() (has bool) { | |||
|
|
|||
| // This function is not declared with a pointer value because otherwise | |||
There was a problem hiding this comment.
This comment should be updated since it's a pointer method now.
Is it safe to use via pointer?
There was a problem hiding this comment.
Yes it is safe, the comment should indeed be updated.
There was a problem hiding this comment.
Just wanted to point out since I was checking back through this that it's safe because of the entry.Dup() call in the new version.
Hooks are now safe to modify entry because any log call on entry duplicates the entire entry.
| if _, err = entry.Logger.Out.Write(serialized); err != nil { | ||
| fmt.Fprintf(os.Stderr, "Failed to write to log, %v\n", err) | ||
| } | ||
| func() { |
There was a problem hiding this comment.
This extra function wrapper isn't required here since the write does nothing else after writing. ie the lock doesn't require it.
There was a problem hiding this comment.
Yes it's a leftover from temporary step, I will remove it
|
@dgsb also noticed the Not sure if this was intentionally left public or not. Might be soon enough to catch if it wasn't intentional. |
|
Good catch we should have a bump on the minor, thanks for your thorough review 👍 |
| for k, v := range entry.Data { | ||
| data[k] = v | ||
| } | ||
| return &Entry{Logger: entry.Logger, Data: data, Time: entry.Time, Context: entry.Context, err: entry.err} |
There was a problem hiding this comment.
Why not copy caller into new entry? There are some scenarios where I want to pass the caller by myself, for example, many libraries accept a logger interface and use it to output internal log(such as SQL exec log for Gorm), so many users wrap logrus to the needed interface, but the caller got by logrus is inaccuracy, and I get the caller and pass it into Entry, and this change breaks my wrapped logger for these libraries.
Can I send a PR for copying caller in Entry.Dup util? What do you think of it? Thanks! @dgsb @epelc
No description provided.